Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service #4623

Merged

Conversation

Pranav-b-7
Copy link
Contributor

@Pranav-b-7 Pranav-b-7 commented Dec 29, 2023

This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for Front50Service.

There are no behaviour changes detected with these code changes and hence no additional tests added to demonstrate the functionalities.
Few existing tests are modified to align it with the new changes.

@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch 2 times, most recently from cfc1b7b to 328d7b8 Compare January 2, 2024 06:58
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch 2 times, most recently from 7fc719e to 483b3e0 Compare January 8, 2024 10:25
@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review January 8, 2024 10:46
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from 483b3e0 to 4d0485f Compare January 9, 2024 06:30
@Pranav-b-7 Pranav-b-7 changed the title refactor(front50): Enhance Exception Handling in Front50Service Retrofit Client refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service Jan 9, 2024
@Pranav-b-7 Pranav-b-7 marked this pull request as draft January 9, 2024 10:36
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch 8 times, most recently from ee64ec5 to d81480a Compare January 12, 2024 11:58
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from 3459464 to fcc2e80 Compare January 23, 2024 10:09
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from fcc2e80 to 29643d8 Compare February 7, 2024 07:49
@Pranav-b-7 Pranav-b-7 closed this Apr 1, 2024
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from 4c6aa53 to 886e2b1 Compare April 1, 2024 14:06
@Pranav-b-7 Pranav-b-7 reopened this Apr 2, 2024
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch 9 times, most recently from 13169ef to ae24d3f Compare April 5, 2024 07:44
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch 2 times, most recently from 5ac2f60 to 59945dd Compare April 5, 2024 11:24
@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review April 5, 2024 11:24
@@ -466,10 +466,9 @@ public boolean hasDisableLock(Moniker clusterMoniker, String account, Location l
Application application;
try {
application = front50Service.get(clusterMoniker.getApp());
} catch (RetrofitError e) {
} catch (SpinnakerHttpException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a test in TrafficGuardSpec that doesn't strictly need to change because of what's happening in this PR. It does look like a broken test though...and there's a unnecessary null check of application a few lines below here. Because Front50Service.get returns a "real" type (i.e. not a Response), it never returns null. So it doesn't make sense to have a test that mocks a null return value. Can you clean this up please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a similar thing happens in UpsertApplicationTaskSpec ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating TrafficGuardSpec. The code below still has a null check that doesn't need to be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating TrafficGuardSpec. The code below still has a null check that doesn't need to be there.

Are you talking about this line : https://github.com/Pranav-b-7/orca/blob/0c986730d3918caf7b9d8509dea45cb426d9d81a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java#L477 ?

If yes, this check is still relevant, because in the above catch block when the response code is 404 or 403, application variable is assigned to null.

@@ -70,10 +71,12 @@ ExecutionImportResponse createExecution(@RequestBody PipelineExecution execution
try {
application = front50Service.get(execution.getApplication());
log.info("Importing application with name: {}", application.name);
} catch (RetrofitError e) {
if (e.getKind() == RetrofitError.Kind.HTTP && e.getResponse().getStatus() != 404) {
} catch (SpinnakerHttpException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have here matches the original logic. I have to wonder if it's really correct though. If the idea is to be silent about 404s, but warn on other things, wouldn't we want to have one catch (SpinnakerServerException) block that logs a warning for anything else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this PR is to move to Spinnaker*Exception, so let's leave the logic as it is for now.

@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from 59945dd to d02575e Compare April 8, 2024 10:52
@Pranav-b-7 Pranav-b-7 closed this Apr 10, 2024
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from 8efeabf to 7e923cc Compare April 10, 2024 08:13
@Pranav-b-7 Pranav-b-7 reopened this Apr 16, 2024
This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for Front50Service.

There are no behaviour changes detected with these code changes and hence no additional tests added to demonstrate the functionalities.
Few existing tests are modified to align it with the new changes.
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from 81dcef8 to ebc0f98 Compare April 16, 2024 06:52
Pranav-b-7 added 4 commits April 16, 2024 12:23
There are 3 APIs invoked inside the try block:
1. Keel Service API - keelService.deleteDeliveryConfig(): With the addition of the commit - c417922, the keelService uses SpinnakerRetrofitErrorHandler.
2. Front50 Service APIs - front50Service.get() and front50Service.delete(): With the addition of the commit - a44b5bb, the front50 service uses SpinnakerRetrofitErrorHandler.

and hence with the above mentioned commits, the catch block with RetrofitError becomes irrelevant.
There are total of 5 API calls happening inside the try block:
1. Igor Service API - scmService.compareCommits(): With the addition of the commit - d430b75, the igorService uses SpinnakerRetrofitErrorHandler.
2. Clouddriver APIs (OortService) - cloudDriverService.getByAmiId() - This is invoked twice in the block, and oortService.getServerGroupFromCluster(): With the addition of the commit - 84a7106, the oortService uses SpinnakerRetrofitErrorHandler.
3. Front50 API - front50Service.get(): With the addition of the commit - a44b5bb, the front50Service uses SpinnakerRetrofitErrorHandler.

and hence with the above mentioned commits, the catch block with RetrofitError becomes irrelevant.
…Spec

The API call 'front50Service.get(application)' mocked/stubbed to return null value is not realistic, because if the application is not available , then this API would throw a 404 HTTP error and thats already handled in the same class: https://github.com/spinnaker/orca/blob/7e923cc05af0f66b952245722aa69c667f685c5c/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy#L464.

and hence this test is cleaned up.
…cationTaskSpec

The API call 'front50Service.get(application)' mocked/stubbed to return null value is not realistic, because if the application is not present, then this API would throw a 404 HTTP error.

and hence this test is cleaned up.
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-Front50Service branch from ebc0f98 to 33f1991 Compare April 16, 2024 06:54
…ic behavior of Front50Service.get

when an application isn't found.  It doesn't return null in this case, it throws an exception.

Restore previously removed "should create an application in global registries" test since
it was providing useful coverage.
@dbyron-sf dbyron-sf added ready to merge Approved and ready for merge and removed ready to merge Approved and ready for merge labels Apr 16, 2024
@mergify mergify bot added the auto merged Merged automatically by a bot label Apr 16, 2024
@mergify mergify bot merged commit 38b447f into spinnaker:master Apr 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.34
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants